Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert config.json to constants #7497

Merged
merged 8 commits into from
Feb 3, 2025

Conversation

olmoh
Copy link
Collaborator

@olmoh olmoh commented Jan 22, 2025

This change is Reviewable

Copy link

linear bot commented Jan 22, 2025

@olmoh olmoh force-pushed the convert-app-config-json-to-typescript-des-1654 branch from d9ce7d5 to 5309c3e Compare January 22, 2025 11:54
@olmoh olmoh requested a review from raksooo January 22, 2025 12:02
@olmoh olmoh marked this pull request as ready for review January 22, 2025 12:02
@olmoh olmoh force-pushed the convert-app-config-json-to-typescript-des-1654 branch from 5309c3e to 6aefdb5 Compare January 22, 2025 16:26
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 86 of 86 files at r1, 27 of 27 files at r2, 15 of 15 files at r3, 7 of 7 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olmoh)


desktop/packages/mullvad-vpn/src/main/index.ts line 860 at r2 (raw file):

      if (Object.values(links).find((link) => url.startsWith(link))) {
        await shell.openExternal(url);
      }

If the links are converted to an enum, wouldn't it be possible to type openUrl to only accept valid links instead of any string? Then this function would not need to check if the link is valid by looking up if url exists in links 😊

Code quote:

    IpcMainEventChannel.app.handleOpenUrl(async (url) => {
      if (Object.values(links).find((link) => url.startsWith(link))) {
        await shell.openExternal(url);
      }

@olmoh olmoh force-pushed the convert-app-config-json-to-typescript-des-1654 branch from 6aefdb5 to 6fb49f8 Compare January 30, 2025 12:50
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 31 of 31 files at r6, 11 of 11 files at r7, 21 of 21 files at r8, 3 of 3 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olmoh)

@olmoh olmoh force-pushed the convert-app-config-json-to-typescript-des-1654 branch from af04961 to 832cdd3 Compare January 31, 2025 10:11
Copy link
Collaborator Author

@olmoh olmoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 56 of 90 files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


desktop/packages/mullvad-vpn/src/main/index.ts line 860 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

If the links are converted to an enum, wouldn't it be possible to type openUrl to only accept valid links instead of any string? Then this function would not need to check if the link is valid by looking up if url exists in links 😊

Great suggestion, I have added typing to the openUrl and related functions 😄

@olmoh olmoh force-pushed the convert-app-config-json-to-typescript-des-1654 branch from 832cdd3 to 9e90fab Compare February 3, 2025 07:45
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 31 of 31 files at r11, 14 of 14 files at r12, 22 of 22 files at r13, 4 of 6 files at r14, 2 of 5 files at r15, 9 of 15 files at r17, 7 of 7 files at r18, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@MarkusPettersson98 MarkusPettersson98 force-pushed the convert-app-config-json-to-typescript-des-1654 branch from 9e90fab to 99b30c4 Compare February 3, 2025 07:56
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r19, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@olmoh olmoh merged commit e0d2a14 into main Feb 3, 2025
11 checks passed
@olmoh olmoh deleted the convert-app-config-json-to-typescript-des-1654 branch February 3, 2025 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants